-
Notifications
You must be signed in to change notification settings - Fork 242
Remove admin client usage from Scala side code #22611
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| (_, participantNames @ ValueList(vs)), | ||
| ), | ||
| ) => | ||
| ) if vs.length <= 1 => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to allow for an empty list case as well as the singleton list case (since daml-script tests sometimes use an empty list)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please (if you havent already) verify this is backwards compatible manually, i.e. build a script dar with some 3.4 version and run it (via dpm script --dar <my-dar> --all --ide-ledger) using HEAD of main
| } | ||
| } yield AllocParty(idHint, allocArg) | ||
| } yield AllocParty(idHint, participantNames.headOption) | ||
| case ValueRecord( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure all other AllocParty payloads result in a non-generic error message
| ) | ||
| .participantUid | ||
| override def getParticipantUid()(implicit ec: ExecutionContext): Future[String] = | ||
| grpcClient.partyManagementClient.getParticipantId().map(_.asInstanceOf[String]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As other methods requiring a participantUid work with a string, we also do the same here
| None, | ||
| PureCompiledPackages.Empty(defaultCompilerConfig), | ||
| ) | ||
| defaultParticipantUid <- ledgerClient.getParticipantUid() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed for vetting
| .map(dep => ScriptLedgerClient.ReadablePackageId.assertFromString(dep.versionedName)) | ||
| .toList | ||
| _ <- ledgerClient.vetPackages(pkgs) | ||
| _ <- ledgerClient.waitUntilVettingVisible(pkgs, defaultParticipantUid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this sufficient? Or do we need to iterate over all participants waiting for vetting visibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if you only wait on one, it might be flaky.
samuel-williams-da
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In PartyManagement.daml, could you revert the AllocateParty datatype back to how it looked before Pauls changes?
i.e.
data AllocateParty = AllocateParty with
requestedName: Text
idHint : Text
participant : Optional Text
instance IsQuestion AllocateParty Party where command = "AllocateParty"instead of how it currently is:
data AllocateParty = AllocateParty with
requestedName: Text
idHint : Text
participants : [Text]
-- | HIDE
instance IsQuestion AllocateParty Party
where
command = "AllocateParty"
version = 2and make sure the comment is correct
Plan is to use a separate PR (#22619) for removing remaining daml side admin client references to multi-participant code